-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add phpDoc support for fully_qualified_strict_types
fixer
#5620
feat: add phpDoc support for fully_qualified_strict_types
fixer
#5620
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Looks interesting! @Jadro007 Any chance for a rebase? 🙂 |
Hello, @Jadro007 you're PR is very interesting, can you rebase ? I would like to use to it in my code. |
e2a96e9
to
0d74c03
Compare
fully_qualified_strict_types
fixer
546f590
to
b787641
Compare
@kubawerlos @keradus (@mvorisek too): second pair of eyes needed 👀. |
Initial work was done in 7acec70 but since the branch became outdated, it couldn't be just rebased - it required significant changes to make it work again. It wasn't even possible to commit not working version and to provide changes afterwards, because there were huge conflicts and keeping the old code would end up with a mess. I decided to fix this up during rebase, so it's working as it was working in proposed PR (still needs improvements, but these will be done separately).
- change `@var` to `@param` where applicable - introduce proper `@var` usages - add annotations with sub-namespace (shortened version contains `\`) - add test case for `@covers` which must be skipped - add empty lines for readability
- Shorten types only in specific allowed annotations - support for sub-namespaces (re-glue tokens instead of using hardcoded first one) - fix tokenizing namespaced string (trailing `\` was added when initial array was filtered)
b787641
to
dbe0e27
Compare
This is a breaking change, and should have been behind config, defaulting to disabled in the current major version series. |
What's breaking here 🤔? The purpose of this fixer is well known, we only covered new places where FQCN can be shortened. |
But the fixer never used to mess with phpdoc comments. When other fixers added new functionality that was totally new, they added config, even when they had a vague description that could be interpreted to include the new functionality already, such as due to new language features. |
The fixer did not touch Importing symbols is something new, though, that's why it's behind the config option. |
Well, I just ran upgraded and had thousands of changes that I wasn't expecting. I'm reverting the code changes on the version I'm using for now, so I can continue to use the features that I want. Even if this is not a BC break, the likes of Laravel framework and the default Laravel app structure wants FQCNs in phpdoc but not elsewhere, so there is a big audience for making this configurable (and a big audience which may consider this change breaking). |
Why Laravel expects FQCNs in phpDoc? This should not make any difference, especially because this fixer requires imports to be in place to be shortened (unless |
A reminder of some docs we have about feature vs bug: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/feature-or-bug.rst With that, it doesn't specify if new feature inside single fixer is consider BC break. It is not - similar to ruleset. With new feature implemented, fixer can cover more and more cases. @Wirone , probably you won't change whatever status-quo of coding standard for whatever ecosystem. (Feel free to deep-dive into reasoning, but don't expect change in it) @GrahamCampbell , if "big audience" wants sth differently, contributions are welcome. |
I am just discussing whether or not this should be behind a config flag at this point. If there is no agreement, there is no point in submitting a PR. I may submit a PR if there is agreement on the approach. |
I don't expect any change, I am just curious about what @GrahamCampbell said 🙂. There is already one exception in the fixer, it does not touch |
OK, if this is a hard no, then Laravel will just continue to operate off of a fork with this PR reverted. |
You know, Laravel may eventually change it's code style, but there would need to be mechanism to still use the old style for older Laravel versions, regardless. |
I don't get how Wirone's "we can cover it" is a "hard no". |
He was asking about specific phpdoc annotations, to which my answer would be all of them, which is equivalent to reverting the PR. |
We can still cover it by introducing the config option as a feature, not as a BC break fix 😉. At any point I wasn't against introducing such opt-out, I was trying to understand why Laravel requires something that does not make sense (at least for me) 😅. |
For what it's worth, I have many projects where the code style is that phpdoc contains the full FQCN, now seeing a big flood of changes like: - /** @var \Stripe\SubscriptionItem $anySubscriptionItem */
+ /** @var SubscriptionItem $anySubscriptionItem */
$anySubscriptionItem = $currentSubscriptionPlan ?? $currentSubscriptionAddons->first(); And also (file is in the root namespace, no *
-* @return \Stripe\StripeClient
+* @return Stripe\StripeClient
*/ Having a way to configure the behavior for phpdoc specifically would be very much appreciated to not have to modify our code style and can continue to use php-cs-fixer latest and greatest (it would be really nice if the full FQCN could even be enforced/fixed in the phpdoc but that might be a feature request all on it's own). |
…ixer (PHP-CS-Fixer#5620)" This reverts commit a5fa4c9.
…ixer (PHP-CS-Fixer#5620)" This reverts commit a5fa4c9.
@stayallive in #7628 I provided a way to configure how phpDocs are handled in terms of FQCN, so you will be able to disable it completely if you don't want to shorten types there. Also, |
…P-CS-Fixer#5620) Co-authored-by: JADRNT01 <jadrnt01@dixonscarphone.com> Co-authored-by: Greg Korba <greg@codito.dev>
Fixes #3958.
I added support to
fully_qualified_strict_types
fixer to be able to work with PHPDoc, as discussed it #3958. This is my first PR to PHP-CS-Fixer, I tried to adhere to the contributing rules, hopefully I didn't miss anything important.There is one small issue probably with integration of some other fixer, I hope we can resolve it.